-
Notifications
You must be signed in to change notification settings - Fork 612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make evm.db not optional #713
Make evm.db not optional #713
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, just a few things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, nits you can ignore
let mut evm = revm::new(); | ||
evm.database(&mut state); | ||
let mut evm = revm::new(Default::default()); | ||
*evm.db() = state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can move this into the new call above
let mut evm = revm::new(); | ||
evm.database(BenchmarkDB::new_bytecode(Bytecode::new())); | ||
let mut evm = revm::new(Default::default()); | ||
*evm.db() = BenchmarkDB::new_bytecode(Bytecode::new()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here and in this file
} | ||
|
||
pub fn new<DB>() -> EVM<DB> { | ||
EVM::new() | ||
pub fn new<DB>(db: DB) -> EVM<DB> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to refresh this in the future, so to not break API two times can you revert this?
pub fn new() -> Self { | ||
Self::with_env(Default::default()) | ||
pub fn new(db: DB) -> Self { | ||
Self::with_env(Default::default(), db) | ||
} | ||
|
||
/// Creates a new [EVM] instance with the given environment. | ||
pub fn with_env(env: Env) -> Self { | ||
Self { env, db: None } | ||
} | ||
|
||
pub fn database(&mut self, db: DB) { | ||
self.db = Some(db); | ||
} | ||
|
||
pub fn db(&mut self) -> Option<&mut DB> { | ||
self.db.as_mut() | ||
pub fn with_env(env: Env, db: DB) -> Self { | ||
Self { env, db } | ||
} | ||
|
||
pub fn take_db(&mut self) -> DB { | ||
core::mem::take(&mut self.db).unwrap() | ||
pub fn db(&mut self) -> &mut DB { | ||
&mut self.db | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert this so as to not break functions
pub fn new() -> Self {
should init with EmptyDB
this would remove a need to have Option
and would make code nicer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks a lot cleaner without Option
, but I would restrain from breaking current functions as this is something I want to rework in near future so it does not make sense to break it twice
Ok I can revert those changes, but I am now struggling because without breaking those functions is not easy for me. In particular I have the problem that, if I delete the impl<DB> EVM<DB> {
/// Creates a new [EVM] instance with the default environment,
pub fn new() -> Self {
Self::with_env(Default::default())
}
/// Creates a new [EVM] instance with the given environment.
pub fn with_env(env: Env) -> Self {
Self { env, db: None }
} ^---> here
pub fn database(&mut self, db: DB) {
self.db = db;
}
pub fn db(&mut self) -> &mut DB {
&mut self.db
}
pub fn take_db(&mut self) -> DB {
core::mem::take(&mut self.db)
} ^---> here
} One way to solve those errors is to add impl<DB: Default> EVM<DB> {
/// Creates a new [EVM] instance with the default environment,
pub fn new() -> Self {
Self::with_env(Default::default())
}
/// Creates a new [EVM] instance with the given environment.
pub fn with_env(env: Env) -> Self {
Self { env, db: Default::default() }
}
pub fn database(&mut self, db: DB) {
self.db = db;
}
pub fn db(&mut self) -> &mut DB {
&mut self.db
}
pub fn take_db(&mut self) -> DB {
core::mem::take(&mut self.db)
}
} But then I have to put the Default constrain also to the So I tried do do as you suggested: init with an impl<DB> EVM<DB> {
/// Creates a new [EVM] instance with the default environment,
pub fn new() -> Self {
Self::with_env(Default::default())
}
/// Creates a new [EVM] instance with the given environment.
pub fn with_env(env: Env) -> Self {
Self { env, db: EmptyDB::default() }
} ^--> here I put EmptyDB
pub fn database(&mut self, db: DB) {
self.db = db;
}
pub fn db(&mut self) -> &mut DB {
&mut self.db
}
pub fn take_db(&mut self) -> DB {
core::mem::take(&mut self.db)
}
} But of course this is not good since the expected type is If you could give me some pointers about how to start tackling this problem I can try and solve it. Thank you very much |
@alessandromazza98 you are right, let us move this to later after we create new revm version |
This is getting changed a lot inside EvmBuilder #888 |
Closes #697
I removed the
Option<DB>
inside theEVM
struct to just leaveDB
.Let me know if it's ok.
Thanks